E2605: Complete frontend support for specialized rubrics by topic#166
Open
PositivelyBookish wants to merge 13 commits into
Open
E2605: Complete frontend support for specialized rubrics by topic#166PositivelyBookish wants to merge 13 commits into
PositivelyBookish wants to merge 13 commits into
Conversation
josev814
reviewed
Apr 3, 2026
added 8 commits
April 22, 2026 08:41
📝 WalkthroughWalkthroughA feature enabling per-topic rubric mappings and rubric variability was added across the Assignments UI and data transformation layers; questionnaire type normalization and question item types were introduced for questionnaires; several tests were added; and minor UI/route/console cleanup plus Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant UI as "TopicsTab / AssignmentEditor (Client)"
participant API as "API (/assignment_questionnaires)"
participant Store as "Client Mappings Cache"
participant Alert as "Alerts"
User->>UI: change topic rubric dropdown
UI->>UI: isTopicRubricMappingPending? (check pending set)
UI->>UI: compute action (POST / PATCH / DELETE)
UI->>API: send request (POST/PATCH/DELETE)
API-->>UI: response (success / error)
UI->>Store: refresh mappings (GET /assignment_questionnaires?assignment_id=...)
Store-->>UI: updated mappings
UI->>Alert: show success or error message
UI-->>User: update dropdown state (disabled/aria-busy cleared)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 2/3 reviews remaining, refill in 20 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Assignments/AssignmentEditor.tsx (1)
1418-1496:⚠️ Potential issue | 🟠 MajorUnreachable dead code should be removed.
There are two
returnstatements in this component. The first return at line 772 renders the tab-based editor UI. This second return statement (lines 1418-1496) rendering a Modal-based UI is unreachable and will never execute.🗑️ Proposed fix: Remove dead code
); - return ( - <Modal size="lg" centered show={true} onHide={handleClose} backdrop="static"> - <Modal.Header closeButton> - <Modal.Title>{mode === "update" ? `Update Assignment - ${assignmentData.name}` : "Create Assignment"}</Modal.Title> - </Modal.Header> - <Modal.Body> - {assignmentError && <p className="text-danger">{assignmentError}</p>} - <Tabs defaultActiveKey="general" id="assignment-tabs"> - <Tab eventKey="general" title="General"> - <Formik - initialValues={mode === "update" ? assignmentData : initialValues} - onSubmit={onSubmit} - validationSchema={validationSchema} - validateOnChange={false} - enableReinitialize={true} - > - {(formik) => { - return ( - <Form> - <FormInput controlId="assignment-name" label="Assignment Name" name="name" /> - ... (entire Modal block) - </Form> - ); - }} - </Formik> - </Tab> - <Tab eventKey="etc" title="Etc"> - <EtcTab assignmentId={assignmentData?.id} /> - </Tab> - </Tabs> - </Modal.Body> - </Modal> - ); }; export default AssignmentEditor;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 1418 - 1496, The second unreachable JSX return in the AssignmentEditor component (the Modal-based UI starting with <Modal size="lg" centered show={true} onHide={handleClose} ...> and ending at </Modal>) should be removed; locate the unused/duplicate return block inside the AssignmentEditor function (the one that renders the Modal and Et cTab) and delete that entire block so only the primary tab-based return remains, and then run the app/build to ensure no missing references (remove any stray variables or imports that were only used by the deleted Modal if the linter flags them).
🧹 Nitpick comments (4)
src/pages/Questionnaires/QuestionnaireUtils.tsx (1)
112-114: Use a shared bidirectional questionnaire-type map.Request transformation normalizes types, but response transformation currently does not. Centralizing both directions avoids backend/UI enum drift.
Refactor sketch
+const BACKEND_TYPE_BY_FORM_TYPE: Record<string, string> = { + Review: "ReviewQuestionnaire", + "Author feedback": "AuthorFeedback", + "Teammate Review": "TeammateReview", + Survey: "Survey", + "Assignment survey": "AssignmentSurvey", + "Global survey": "GlobalSurvey", + "Course survey": "CourseSurvey", + "Bookmark rating": "BookmarkRating", + Quiz: "Quiz", +}; + +const FORM_TYPE_BY_BACKEND_TYPE = Object.fromEntries( + Object.entries(BACKEND_TYPE_BY_FORM_TYPE).map(([k, v]) => [v, k]) +); + export const transformQuestionnaireRequest = (values: QuestionnaireFormValues) => { const questionnaire: QuestionnaireRequest = { @@ - questionnaire_type: values.questionnaire_type === "Review" - ? "ReviewQuestionnaire" - : values.questionnaire_type.replace(/\s+/g, ""), + questionnaire_type: + BACKEND_TYPE_BY_FORM_TYPE[values.questionnaire_type] ?? + values.questionnaire_type.replace(/\s+/g, ""), @@ export const transformQuestionnaireResponse = (data: any): QuestionnaireFormValues => { return { @@ - questionnaire_type: data.questionnaire_type, + questionnaire_type: + FORM_TYPE_BY_BACKEND_TYPE[data.questionnaire_type] ?? data.questionnaire_type,Also applies to: 131-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnaireUtils.tsx` around lines 112 - 114, The code normalizes questionnaire_type on request using a custom replace/ternary but lacks a shared bidirectional map for request/response leading to enum drift; create a single mapping object (e.g., QUESTIONNAIRE_TYPE_MAP with keys as UI labels and values as backend enums and derive the inverse map) and replace the inline logic in the request transform (where questionnaire_type is set from values.questionnaire_type) and the response transform (the block around lines 131-137) to use this shared map and its inverse so both directions consistently translate types.src/pages/Assignments/AssignmentEditor.test.tsx (1)
227-250: Consider adding test coverage for the default/fallback case.The test verifies that
vary_by_topic: trueis correctly mapped, but there's no assertion thatreview_rubric_varies_by_topicdefaults tofalsewhenvary_by_topicis absent from the backend response. This default behavior is mentioned in the summary and implemented intransformAssignmentResponse.💡 Suggested additional test case
expect(values.review_rubric_varies_by_topic).toBe(true); }); + + it("defaults review_rubric_varies_by_topic to false when vary_by_topic is absent", () => { + const assignment = { + id: 1, + name: "Test Assignment", + directory_path: "assignment_1", + spec_location: "http://example.com", + private: false, + show_template_review: false, + require_quiz: false, + has_badge: false, + staggered_deadline: false, + is_calibrated: false, + num_review_rounds: 1, + due_dates: [], + assignment_questionnaires: [], + }; + + const values = transformAssignmentResponse(JSON.stringify(assignment)); + + expect(values.review_rubric_varies_by_topic).toBe(false); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/AssignmentEditor.test.tsx` around lines 227 - 250, Add a test to cover the default/fallback behavior when vary_by_topic is missing: call transformAssignmentResponse with an assignment object that does NOT include the vary_by_topic property (e.g., omit that key from the payload or delete it) and assert that the returned values.review_rubric_varies_by_topic is false; reference the existing test block for transformAssignmentResponse and mirror its structure (use JSON.stringify on the assignment payload and the same expect style) to ensure the default mapping is verified.src/pages/Assignments/tabs/TopicsTab.tsx (2)
343-350: DuplicatefindTopicRubricMappingimplementation.This function has an identical implementation in
AssignmentEditor.tsx(lines 535-540). While TopicsTab needs it for rendering and AssignmentEditor needs it for the change handler, consider whether one component could pass the resolved mapping value directly to avoid the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/tabs/TopicsTab.tsx` around lines 343 - 350, The findTopicRubricMapping implementation is duplicated between TopicsTab and AssignmentEditor; remove the duplication by moving the logic up to a common owner or by having the component that already computes it pass the resolved value down. Specifically, replace the local findTopicRubricMapping in TopicsTab with a prop (e.g., topicRubricMapping or a getTopicRubricMapping function) provided by the parent or by AssignmentEditor, using the same matching logic against topicRubricMappings (compare Number(mapping.project_topic_id) to Number(topicDatabaseId) and compare used_in_round with usedInRound via null-coalescing). Update TopicsTab usage sites to consume the passed mapping or call the single shared getTopicRubricMapping so only one implementation remains.
59-70: Consider extracting shared types to avoid duplication.The
TopicRubricMappinginterface is duplicated in bothTopicsTab.tsxandAssignmentEditor.tsx. Consider extracting it to a shared location (e.g.,interfaces.tsor a dedicated types file) to maintain a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/tabs/TopicsTab.tsx` around lines 59 - 70, The TopicRubricMapping interface is duplicated; extract it (and RubricOption if also reused) into a shared types file (e.g., create a new interfaces/types module), export TopicRubricMapping from that module, update TopicsTab.tsx and AssignmentEditor.tsx to import TopicRubricMapping (and RubricOption) instead of declaring them locally, remove the duplicate declarations, and run TypeScript checks to ensure no unresolved symbols remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Questionnaires/QuestionnaireForm.tsx`:
- Around line 13-15: availableItemTypes currently maps itemTypes.data twice and
duplicates work; change the logic in the availableItemTypes constant to compute
the mapped array once (e.g., assign const mapped = itemTypes?.data?.map((t: any)
=> t.name) as string[] and then use mapped.length ? mapped : QuestionItemTypes)
so the mapping is not performed twice, and remove the debug console.log that
prints fetched data during render (the debug logging referenced near line 21) to
avoid logging in the render lifecycle.
In `@src/pages/Questionnaires/QuestionnaireUtils.tsx`:
- Line 108: Remove the two noisy debug console.log statements that print full
form/request objects (specifically the console.log("Original Form Values:",
values) call and the similar log at line 127) so sensitive payloads are not
exposed; either delete these console.log calls or replace them with a sanitized
or conditional debug log (use a dedicated logger or an isDebug flag and redact
PII) and update any tests or usages relying on those logs; search for the exact
string "Original Form Values:" and the corresponding request object log in
QuestionnaireUtils.tsx to locate and remove/replace them.
In `@src/pages/Reviews/reviews.tsx`:
- Around line 88-105: The effect calling fetchResolvedRubric should clear stale
state and handle the returned promise to avoid unhandled rejections: when
responseMapId or reviewRound changes, immediately setResolvedRubric(null) (or
undefined) to clear the previous rubric, then invoke fetchResolvedRubric inside
an async IIFE or promise chain and wrap it in try/catch so any thrown error from
fetchResolvedRubric is caught and handled (e.g., log it or set an error state)
instead of bubbling as an unhandled rejection; keep the second effect that syncs
rubricResponse.data to setResolvedRubric but ensure you don’t overwrite the
cleared state on error.
---
Outside diff comments:
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 1418-1496: The second unreachable JSX return in the
AssignmentEditor component (the Modal-based UI starting with <Modal size="lg"
centered show={true} onHide={handleClose} ...> and ending at </Modal>) should be
removed; locate the unused/duplicate return block inside the AssignmentEditor
function (the one that renders the Modal and Et cTab) and delete that entire
block so only the primary tab-based return remains, and then run the app/build
to ensure no missing references (remove any stray variables or imports that were
only used by the deleted Modal if the linter flags them).
---
Nitpick comments:
In `@src/pages/Assignments/AssignmentEditor.test.tsx`:
- Around line 227-250: Add a test to cover the default/fallback behavior when
vary_by_topic is missing: call transformAssignmentResponse with an assignment
object that does NOT include the vary_by_topic property (e.g., omit that key
from the payload or delete it) and assert that the returned
values.review_rubric_varies_by_topic is false; reference the existing test block
for transformAssignmentResponse and mirror its structure (use JSON.stringify on
the assignment payload and the same expect style) to ensure the default mapping
is verified.
In `@src/pages/Assignments/tabs/TopicsTab.tsx`:
- Around line 343-350: The findTopicRubricMapping implementation is duplicated
between TopicsTab and AssignmentEditor; remove the duplication by moving the
logic up to a common owner or by having the component that already computes it
pass the resolved value down. Specifically, replace the local
findTopicRubricMapping in TopicsTab with a prop (e.g., topicRubricMapping or a
getTopicRubricMapping function) provided by the parent or by AssignmentEditor,
using the same matching logic against topicRubricMappings (compare
Number(mapping.project_topic_id) to Number(topicDatabaseId) and compare
used_in_round with usedInRound via null-coalescing). Update TopicsTab usage
sites to consume the passed mapping or call the single shared
getTopicRubricMapping so only one implementation remains.
- Around line 59-70: The TopicRubricMapping interface is duplicated; extract it
(and RubricOption if also reused) into a shared types file (e.g., create a new
interfaces/types module), export TopicRubricMapping from that module, update
TopicsTab.tsx and AssignmentEditor.tsx to import TopicRubricMapping (and
RubricOption) instead of declaring them locally, remove the duplicate
declarations, and run TypeScript checks to ensure no unresolved symbols remain.
In `@src/pages/Questionnaires/QuestionnaireUtils.tsx`:
- Around line 112-114: The code normalizes questionnaire_type on request using a
custom replace/ternary but lacks a shared bidirectional map for request/response
leading to enum drift; create a single mapping object (e.g.,
QUESTIONNAIRE_TYPE_MAP with keys as UI labels and values as backend enums and
derive the inverse map) and replace the inline logic in the request transform
(where questionnaire_type is set from values.questionnaire_type) and the
response transform (the block around lines 131-137) to use this shared map and
its inverse so both directions consistently translate types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d4ced0f-4a68-4967-8168-e187c4c0f5eb
📒 Files selected for processing (12)
.gitignoresrc/pages/Assignments/AssignmentEditor.test.tsxsrc/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/AssignmentUtil.tssrc/pages/Assignments/tabs/TopicsTab.test.tsxsrc/pages/Assignments/tabs/TopicsTab.tsxsrc/pages/Questionnaires/Questionnaire.tsxsrc/pages/Questionnaires/QuestionnaireForm.tsxsrc/pages/Questionnaires/QuestionnaireUtils.test.tssrc/pages/Questionnaires/QuestionnaireUtils.tsxsrc/pages/Reviews/reviews.tsxsrc/utils/interfaces.ts
Comment on lines
+13
to
+15
| const availableItemTypes = (itemTypes?.data?.map((t: any) => t.name) as string[])?.length | ||
| ? (itemTypes.data.map((t: any) => t.name) as string[]) | ||
| : QuestionItemTypes; |
There was a problem hiding this comment.
Deduplicate item-type mapping and remove debug logging.
Line 13–15 maps the same array twice; Line 21 logs fetched data during render lifecycle.
Suggested fix
- const availableItemTypes = (itemTypes?.data?.map((t: any) => t.name) as string[])?.length
- ? (itemTypes.data.map((t: any) => t.name) as string[])
- : QuestionItemTypes;
+ const mappedItemTypes = Array.isArray(itemTypes?.data)
+ ? (itemTypes.data.map((t: any) => t.name) as string[])
+ : [];
+ const availableItemTypes = mappedItemTypes.length ? mappedItemTypes : QuestionItemTypes;
@@
- fetchItemTypes({ url: "/questions/types" });
- console.log(itemTypes?.data);
+ fetchItemTypes({ url: "/questions/types" });
}, [fetchItemTypes]);Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Questionnaires/QuestionnaireForm.tsx` around lines 13 - 15,
availableItemTypes currently maps itemTypes.data twice and duplicates work;
change the logic in the availableItemTypes constant to compute the mapped array
once (e.g., assign const mapped = itemTypes?.data?.map((t: any) => t.name) as
string[] and then use mapped.length ? mapped : QuestionItemTypes) so the mapping
is not performed twice, and remove the debug console.log that prints fetched
data during render (the debug logging referenced near line 21) to avoid logging
in the render lifecycle.
Comment on lines
+88
to
+105
| useEffect(() => { | ||
| if (!responseMapId) return; | ||
|
|
||
| fetchResolvedRubric({ | ||
| url: "/student_tasks/rubric_for", | ||
| method: "GET", | ||
| params: { | ||
| response_map_id: responseMapId, | ||
| ...(reviewRound ? { round: reviewRound } : {}), | ||
| }, | ||
| }); | ||
| }, [fetchResolvedRubric, responseMapId, reviewRound]); | ||
|
|
||
| useEffect(() => { | ||
| if (rubricResponse?.data) { | ||
| setResolvedRubric(rubricResponse.data); | ||
| } | ||
| }, [rubricResponse]); |
There was a problem hiding this comment.
Handle the fetch promise and clear stale rubric state.
fetchResolvedRubric rethrows on failure via useAPI, but this effect neither awaits nor catches the promise, so a failed lookup will surface as an unhandled rejection. It also leaves resolvedRubric populated until the next response arrives, which can briefly show the previous rubric after response_map_id or round changes.
♻️ Proposed fix
useEffect(() => {
+ setResolvedRubric(null);
if (!responseMapId) return;
- fetchResolvedRubric({
+ void fetchResolvedRubric({
url: "/student_tasks/rubric_for",
method: "GET",
params: {
response_map_id: responseMapId,
...(reviewRound ? { round: reviewRound } : {}),
},
- });
+ }).catch(() => {});
}, [fetchResolvedRubric, responseMapId, reviewRound]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!responseMapId) return; | |
| fetchResolvedRubric({ | |
| url: "/student_tasks/rubric_for", | |
| method: "GET", | |
| params: { | |
| response_map_id: responseMapId, | |
| ...(reviewRound ? { round: reviewRound } : {}), | |
| }, | |
| }); | |
| }, [fetchResolvedRubric, responseMapId, reviewRound]); | |
| useEffect(() => { | |
| if (rubricResponse?.data) { | |
| setResolvedRubric(rubricResponse.data); | |
| } | |
| }, [rubricResponse]); | |
| useEffect(() => { | |
| setResolvedRubric(null); | |
| if (!responseMapId) return; | |
| void fetchResolvedRubric({ | |
| url: "/student_tasks/rubric_for", | |
| method: "GET", | |
| params: { | |
| response_map_id: responseMapId, | |
| ...(reviewRound ? { round: reviewRound } : {}), | |
| }, | |
| }).catch(() => {}); | |
| }, [fetchResolvedRubric, responseMapId, reviewRound]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Reviews/reviews.tsx` around lines 88 - 105, The effect calling
fetchResolvedRubric should clear stale state and handle the returned promise to
avoid unhandled rejections: when responseMapId or reviewRound changes,
immediately setResolvedRubric(null) (or undefined) to clear the previous rubric,
then invoke fetchResolvedRubric inside an async IIFE or promise chain and wrap
it in try/catch so any thrown error from fetchResolvedRubric is caught and
handled (e.g., log it or set an error state) instead of bubbling as an unhandled
rejection; keep the second effect that syncs rubricResponse.data to
setResolvedRubric but ensure you don’t overwrite the cleared state on error.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/pages/Reviews/reviews.tsx (1)
87-104:⚠️ Potential issue | 🟠 MajorHandle async failures and clear stale rubric before each lookup.
At Line 87-Line 98,
fetchResolvedRubric(...)is fired without handling the returned promise, and previousresolvedRubricis not cleared before a new request. SinceuseAPI.sendRequestrethrows, this can produce unhandled rejections and stale rubric display until the next response.♻️ Suggested fix
useEffect(() => { + setResolvedRubric(null); if (!responseMapId) return; - fetchResolvedRubric({ + void fetchResolvedRubric({ url: "/student_tasks/rubric_for", method: "GET", params: { response_map_id: responseMapId, ...(reviewRound ? { round: reviewRound } : {}), }, - }); + }).catch(() => { + // keep rubric cleared; error is already exposed through rubricError + }); }, [fetchResolvedRubric, responseMapId, reviewRound]);#!/bin/bash # Verify the risk: # 1) useAPI rethrows request failures # 2) this effect does not catch/void-handle the promise rg -nP 'throw err' src/hooks/useAPI.ts rg -nP 'useEffect\(\(\)\s*=>\s*\{[\s\S]{0,400}fetchResolvedRubric\(' src/pages/Reviews/reviews.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Reviews/reviews.tsx` around lines 87 - 104, Before firing fetchResolvedRubric, clear the previous resolvedRubric and avoid unhandled rejections by awaiting the request inside an async function and catching errors; specifically, in the effect that currently calls fetchResolvedRubric, call setResolvedRubric(null) (or equivalent) then run an async IIFE that awaits fetchResolvedRubric({...}) and wraps it in try/catch to handle/log the error (or swallow it), and keep the existing separate effect that sets resolvedRubric from rubricResponse as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 954-974: The rubric fields must share one stable row key instead
of using row.original.id in some places; create a single derived rowKey (e.g.,
if row.original.title === "Author feedback:" use "author_feedback", else if
"Teammate review:" use "teammate_review", otherwise use `round_${row.original.id
|| 0}`) and replace all usages of questionnaireFieldName/questionnaireControlId
and the sibling weight and notification-limit name/controlId generation to use
that rowKey (update FormSelect's name/controlId and the corresponding weight and
notification-limit Formik paths) so the questionnaire, weight, and
notification-limit fields use the same stable identifier across the component.
- Around line 538-585: handleTopicRubricChange reads existingMapping before the
async save which can be stale and cause duplicate POSTs or missed DELETEs; fix
by making the mutation update topicRubricMappings optimistically or by locking
the selector until the change completes. Specifically, when calling
saveTopicRubricMapping in handleTopicRubricChange, immediately update the
in-memory topicRubricMappings (or set a pending map keyed by topicDatabaseId) to
reflect the expected result from the POST/PATCH/DELETE so subsequent rapid
invocations use the updated state instead of findTopicRubricMapping’s stale
value, and clear the pending/optimistic update when refreshTopicRubricMappings
resolves or when the API returns an error.
In `@src/pages/Questionnaires/QuestionnaireUtils.tsx`:
- Around line 129-135: transformQuestionnaireResponse currently returns
questionnaire_type unchanged; add inbound normalization to reverse the outbound
mapping (so UI-facing values match form options). In
transformQuestionnaireResponse map backend value "ReviewQuestionnaire" back to
"Review" (and preserve other values unchanged) for the questionnaire_type field
so edit forms receive the same option keys that the UI expects.
---
Duplicate comments:
In `@src/pages/Reviews/reviews.tsx`:
- Around line 87-104: Before firing fetchResolvedRubric, clear the previous
resolvedRubric and avoid unhandled rejections by awaiting the request inside an
async function and catching errors; specifically, in the effect that currently
calls fetchResolvedRubric, call setResolvedRubric(null) (or equivalent) then run
an async IIFE that awaits fetchResolvedRubric({...}) and wraps it in try/catch
to handle/log the error (or swallow it), and keep the existing separate effect
that sets resolvedRubric from rubricResponse as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efcaef48-4634-4aa2-bd6c-6049a1d923d6
📒 Files selected for processing (6)
src/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/tabs/TopicsTab.tsxsrc/pages/Questionnaires/Questionnaire.tsxsrc/pages/Questionnaires/QuestionnaireForm.tsxsrc/pages/Questionnaires/QuestionnaireUtils.tsxsrc/pages/Reviews/reviews.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Questionnaires/Questionnaire.tsx
- src/pages/Questionnaires/QuestionnaireForm.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 993-1002: The two FormSelect controls in AssignmentEditor that set
questionnaireFieldName to "author_feedback_questionnaire" and
"teammate_review_questionnaire" are using keys that transformAssignmentRequest
does not serialize (it expects questionnaire_round_${i} and writes into
assignment_questionnaires_attributes); update either the control names or the
serializer: change the names set in AssignmentEditor (where FormSelect uses
questionnaireFieldName) to follow the questionnaire_round_${i} naming convention
used by transformAssignmentRequest, or modify transformAssignmentRequest (and
any response mapping) to accept and emit the new keys
("author_feedback_questionnaire" and "teammate_review_questionnaire") and ensure
they are merged into assignment_questionnaires_attributes so selections persist
on save/reload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4622883c-91f2-4f09-b5a9-0863d78f8b9e
📒 Files selected for processing (6)
src/pages/Assignments/AssignmentEditor.test.tsxsrc/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/tabs/TopicsTab.test.tsxsrc/pages/Assignments/tabs/TopicsTab.tsxsrc/pages/Questionnaires/QuestionnaireUtils.test.tssrc/pages/Questionnaires/QuestionnaireUtils.tsx
✅ Files skipped from review due to trivial changes (1)
- src/pages/Questionnaires/QuestionnaireUtils.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Assignments/tabs/TopicsTab.test.tsx
- src/pages/Assignments/AssignmentEditor.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pages/Assignments/AssignmentEditor.test.tsx (1)
106-138: ⚡ Quick winExercise the special-rubric selects through the rendered UI, not just their field names.
This test only checks DOM ids/names. It would still pass if the comboboxes emitted label strings while the submit path expects numeric
questionnaire_idvalues. A render/select/submit assertion for the Author feedback and Teammate review rows would cover the real regression surface here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/AssignmentEditor.test.tsx` around lines 106 - 138, Update the test to actually interact with the rendered selects and submit the form so we assert numeric questionnaire_id values are sent: use render(<AssignmentEditor ...>), locate the Author feedback and Teammate review comboboxes via getRow(...) and within(...).getByRole("combobox"), change their selected value to a real numeric questionnaire id (e.g. by firing a change event with a numeric value), submit the editor form (trigger the submit handler or click the save button), and assert the submit payload contains questionnaire_id fields with the expected numeric values for AUTHOR_FEEDBACK_RUBRIC_ROW_KEY and TEAMMATE_REVIEW_RUBRIC_ROW_KEY rather than string labels; keep the existing id/name assertions (expectRubricFields) and add the render/select/submit assertions using the same helpers and constants (AssignmentEditor, getRow, AUTHOR_FEEDBACK_QUESTIONNAIRE_FIELD, TEAMMATE_REVIEW_QUESTIONNAIRE_FIELD, AUTHOR_FEEDBACK_RUBRIC_ROW_KEY, TEAMMATE_REVIEW_RUBRIC_ROW_KEY).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 776-783: The prefill uses numeric questionnaire ids via
getSpecialRubricQuestionnaireField and formInitialValues but the Author feedback
/ Teammate review dropdowns supply string label values, breaking update-mode and
causing transformAssignmentRequest to send a string questionnaire_id; fix by
changing those dropdown options to objects { label: q.name, value: q.id } (same
pattern used by other rubric selectors) and ensure the select components for the
Author feedback and Teammate review fields read/write the value as the numeric
id (so formInitialValues[specialRubricField.questionnaireField] matches
option.value) and update transformAssignmentRequest to expect/send the numeric
id from those selects.
- Around line 756-761: The review-rubric rows in the Rubrics table still use the
generic questionnaireOptions, allowing non-review questionnaires to be saved;
change those rows to use the filtered reviewRubricOptions instead. Locate where
the Rubrics table renders the review-round/default rows (the code that currently
references questionnaireOptions) and replace the data source/prop with
reviewRubricOptions so only items from the review-questionnaire filter are
selectable/saved; also ensure any prop passed down to TopicsTab or its child
components that represent review-rubric choices uses reviewRubricOptions (not
questionnaireOptions) to keep behavior consistent.
---
Nitpick comments:
In `@src/pages/Assignments/AssignmentEditor.test.tsx`:
- Around line 106-138: Update the test to actually interact with the rendered
selects and submit the form so we assert numeric questionnaire_id values are
sent: use render(<AssignmentEditor ...>), locate the Author feedback and
Teammate review comboboxes via getRow(...) and
within(...).getByRole("combobox"), change their selected value to a real numeric
questionnaire id (e.g. by firing a change event with a numeric value), submit
the editor form (trigger the submit handler or click the save button), and
assert the submit payload contains questionnaire_id fields with the expected
numeric values for AUTHOR_FEEDBACK_RUBRIC_ROW_KEY and
TEAMMATE_REVIEW_RUBRIC_ROW_KEY rather than string labels; keep the existing
id/name assertions (expectRubricFields) and add the render/select/submit
assertions using the same helpers and constants (AssignmentEditor, getRow,
AUTHOR_FEEDBACK_QUESTIONNAIRE_FIELD, TEAMMATE_REVIEW_QUESTIONNAIRE_FIELD,
AUTHOR_FEEDBACK_RUBRIC_ROW_KEY, TEAMMATE_REVIEW_RUBRIC_ROW_KEY).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf838922-97a5-4c8a-9348-bdf256c1f760
📒 Files selected for processing (3)
src/pages/Assignments/AssignmentEditor.test.tsxsrc/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/AssignmentUtil.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Assignments/AssignmentUtil.ts
Comment on lines
+756
to
+761
| const reviewRubricOptions = (assignmentData.questionnaires || []) | ||
| .filter((q: any) => q.questionnaire_type === "ReviewQuestionnaire") | ||
| .map((q: any) => ({ | ||
| label: q.name, | ||
| value: q.id, | ||
| })); |
There was a problem hiding this comment.
Use reviewRubricOptions for the assignment review-rubric rows too.
This filtered list only gets passed into TopicsTab; the review-round/default rows in the Rubrics table still read from questionnaireOptions, so non-review questionnaires can still be saved as the assignment’s review rubric once they appear in assignmentData.questionnaires.
Suggested patch
{
id: i + 1,
rowKey: i + 1,
title: `Review round ${i + 1}:`,
- questionnaire_options: questionnaireOptions,
+ questionnaire_options: reviewRubricOptions,
selected_questionnaire: roundSelections[i + 1]?.id,
questionnaire_type: 'dropdown',
},
...
{
id: 0,
rowKey: 0,
title: "Review rubric:",
- questionnaire_options: questionnaireOptions,
+ questionnaire_options: reviewRubricOptions,
selected_questionnaire: roundSelections[1]?.id,
questionnaire_type: 'dropdown',
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 756 - 761, The
review-rubric rows in the Rubrics table still use the generic
questionnaireOptions, allowing non-review questionnaires to be saved; change
those rows to use the filtered reviewRubricOptions instead. Locate where the
Rubrics table renders the review-round/default rows (the code that currently
references questionnaireOptions) and replace the data source/prop with
reviewRubricOptions so only items from the review-questionnaire filter are
selectable/saved; also ensure any prop passed down to TopicsTab or its child
components that represent review-rubric choices uses reviewRubricOptions (not
questionnaireOptions) to keep behavior consistent.
Comment on lines
+776
to
+783
| const specialRubricField = getSpecialRubricQuestionnaireField( | ||
| aq.used_in_round, | ||
| aq.questionnaire?.questionnaire_type ?? aq.questionnaire_type | ||
| ); | ||
|
|
||
| if (specialRubricField) { | ||
| formInitialValues[specialRubricField.questionnaireField] = questionnaireId; | ||
| formInitialValues[specialRubricField.assignmentQuestionnaireIdField] = aq.id; |
There was a problem hiding this comment.
Keep the special-rubric field values aligned with the select option values.
These fields are now prefilled with numeric questionnaire ids, but the Author feedback / Teammate review dropdowns still use label strings as their option value. That breaks update-mode prefill and causes transformAssignmentRequest to send a string questionnaire_id after a user change instead of the backend questionnaire id.
Please back those dropdowns with { label: q.name, value: q.id } options from the API, the same way the other rubric selectors are handled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 776 - 783, The
prefill uses numeric questionnaire ids via getSpecialRubricQuestionnaireField
and formInitialValues but the Author feedback / Teammate review dropdowns supply
string label values, breaking update-mode and causing transformAssignmentRequest
to send a string questionnaire_id; fix by changing those dropdown options to
objects { label: q.name, value: q.id } (same pattern used by other rubric
selectors) and ensure the select components for the Author feedback and Teammate
review fields read/write the value as the numeric id (so
formInitialValues[specialRubricField.questionnaireField] matches option.value)
and update transformAssignmentRequest to expect/send the numeric id from those
selects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR completes the frontend support for specialized rubrics by topic in the assignment editor.
It adds the UI and request handling needed for instructors to:
Review rubric varies by topic?TopicstabChanges
vary_by_topicstate in the assignment editorTopicstab when topic-specific rubrics are enabledBehavior
After this change, the assignment editor supports all expected rubric-selection cases:
When
Review rubric varies by topic?is enabled:Manual verification:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores